Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding property mapper for product eav attribute -> search weight. #17668

Conversation

bartoszkubicki
Copy link

@bartoszkubicki bartoszkubicki commented Aug 17, 2018

Description

This PR creates property mapper of type Magento\Eav\Model\Entity\Setup\PropertyMapperAbstract to make sure, while adding new product attribute programatically its search weight is persisted into db. Apart of it, unit test for property mapper provided.

Fixed Issues (if relevant)

Manual testing scenarios

  1. Add new attribute for products programatically, setting search weight and method magento/module-eav/Setup/EavSetup.php:addAttribute()
  2. Make sure search weight is persisted into db and it is not its default value.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Aug 17, 2018

CLA assistant check
All committers have signed the CLA.

@magento-engcom-team
Copy link
Contributor

Hi @bartek9007. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@bartoszkubicki
Copy link
Author

bartoszkubicki commented Aug 19, 2018

@magento-engcom-team
Should I include into PRs, strict types declaration and type hinting?

Code quality check showed that I have unused param, but it is required by interface implemented by parent class. Other implementations also not necessarily uses this param.
Errors from code sniffer are fixed.

@ishakhsuvarov ishakhsuvarov added this to the Release: 2.3.1 milestone Sep 18, 2018
@josefbehr
Copy link
Contributor

josefbehr commented Oct 6, 2018

@bartoszkubicki Thank you for your contribution. Can you please fix the failing tests? The unused parameter can probably be ignored.
edit: Is this a known issue that you fixed, or did you find the problem yourself? How did it come about? I'm not sure what problem this fixes.

@bartoszkubicki
Copy link
Author

@josefbehr How it can be ommited if property mappers should implement \Magento\Eav\Model\Entity\Setup\PropertyMapperInterface which has this argument required public function map(array $input, $entityTypeId);

@josefbehr
Copy link
Contributor

@bartoszkubicki I meant you can ignore the warning about the unused parameter, not remove it :) What about my other questions?

@bartoszkubicki
Copy link
Author

@josefbehr I have added some changes, I hope that test failures will be gone.

@josefbehr
Copy link
Contributor

@bartoszkubicki Looking good, thanks. Can you please answer my other questions, and suppress the warning about the unused variable?

@bartoszkubicki
Copy link
Author

bartoszkubicki commented Oct 8, 2018

@josefbehr Didn't notice your edit. I have faced this problem by myself. When adding product attribute programatically, search weight of attributes wasn't persisted to db (table catalog_eav_attribute), because of lacking mapping. Firstly I have fixed it in my current project, the decided to create pull request. I haven't found any issue about that - problem is not very serious, but tend to be annoying when you have to configure a lot of product attributes, and later you have to adjust search weight to requested values, despite of setting in initial script.

Adding property mapper for product eav attribute -> search weight - cs fixes.

Adding return types.

Adding return types.

Adding return types.
@magento-engcom-team
Copy link
Contributor

Hi @josefbehr, thank you for the review.
ENGCOM-4358 has been created to process this Pull Request

@soleksii
Copy link

soleksii commented Mar 5, 2019

✔️ QA Passed

@nmalevanec nmalevanec self-assigned this Mar 5, 2019
@magento-engcom-team magento-engcom-team merged commit 83fa42d into magento:2.3-develop Mar 8, 2019
@ghost
Copy link

ghost commented Mar 8, 2019

Hi @bartoszkubicki, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.2 milestone Mar 8, 2019
@bartoszkubicki bartoszkubicki deleted the search-weight-property-mapper branch June 25, 2019 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants